Add support for the default MemCacheStore from ActiveSupport #465
Conversation
There was a problem hiding this comment.
Also if this PR is merged, the README could be updated to advise on using Dalli, but I'd need to check on the configuration options.
👍. I think we mainly just need to recommend using failover: false which sounds like the same thing as auto_eject_hosts: false that we currently recommend for MemcachedStore.
| @@ -12,6 +12,10 @@ def initialize(cache_adaptor = nil) | |||
| end | |||
|
|
|||
| def cache_backend=(cache_adaptor) | |||
| if cache_adaptor.class.name == 'ActiveSupport::Cache::MemCacheStore' | |||
| cache_adaptor.extend(MemCacheStoreCAS) | |||
There was a problem hiding this comment.
Should we wrap the cache adapter rather than mutating it with extend? Since ideally we would add CAS support to ActiveSupport::Cache::MemCacheStore itself, in which case we could have a conflict from this extension affecting code using the adapter independently of this library.
It means we would need to get the underlying dalli client using cache_store.instance_variable_get(:@data), but the coupling would be the same.
@rafaelfranca should ActiveSupport::Cache::MemCacheStore expose the underlying dalli client? That would be consistent with the database adapters which expose the underlying client through raw_connection.
There was a problem hiding this comment.
Should we wrap the cache adapter rather than mutating it with
extend?
I thought about that, but we need to access a bunch of private methods like merged_options, etc. So it makes the code much more terse.
Since ideally we would add CAS support to ActiveSupport::Cache::MemCacheStore itself
ActiveSupport::Cache has no CAS support, but maybe it should, it could be worth starting a discussion upstream, since other backends such as Redis could support it. However I'm not sure the interface would be the same anyway.
There was a problem hiding this comment.
I'm not sure the interface would be the same anyway.
Yeah, the fact that the interface might not be the same is why I'm thinking it being implemented upstream could cause a conflict.
I thought about that, but we need to access a bunch of private methods like
merged_options, etc. So it makes the code much more terse.
Good point. Maybe this is a good use case for a refinement.
There was a problem hiding this comment.
Maybe this is a good use case for a refinement.
Oh, good idea, I never think of that.
There was a problem hiding this comment.
Actually, not sure if a refinement would be very practical, as we'd have to conditionally activate it. But we only know if it's useful much later at runtime. And nothing guarantees than checking for defined? AS::Cache::MemCacheStore at load time would be accurate.
There was a problem hiding this comment.
Oh right, refinements are statically scoped, which doesn't work as well for extending an optional dependency. Maybe we should just raise if the AS::Cache::MemCacheStore object responds to cas or cas_multi, then handle that compatibility in this library if/when CAS support is added upstream.
There was a problem hiding this comment.
Yeah, that's probably the best idea.
Anyway if cas make it to AS::Cache, it's likely one of us who'll PR it so I think we'll know :)
casperisfine
force-pushed
the
dalli-support
branch
from
69480f2 to
ac7866c
Compare
casperisfine
force-pushed
the
dalli-support
branch
2 times, most recently
from
ee09ad9 to
1b3dcf6
Compare
casperisfine
force-pushed
the
dalli-support
branch
from
1b3dcf6 to
3539264
Compare
|
Not really related with IDC itself, but another thing that is lost when using the Dalli based I'm trying to see if I can refactor |
|
Any plans to release a new version with this integration? We need the integration but don't want to use |
|
It's true that we didn't have a release in quite a long time. But I'm not too sure how stable master is. @dylanahsmith what do you think? |
|
I think master is fairly stable at the moment and we should probably make a release. |
Long story short,
memcachedis getting increasingly problematic, it's not really well maintained, and anyway it's based on the deprecatedlibmemcached.Most newer app uses
dallithrough Rails built-inMemCacheStorebackend, so supporting it is very appealing. You might know that the binary protocol dalli uses is about to be deprecated as well in favor of the new "meta commands" protocol, but I'm fairly confident the Dalli can be converted to that new protocol just fine.So I tried it, and it was surprisingly easy. The only downside was that I had to extend
MemCacheStore, but that was to be expected.Also if this PR is merged, the
READMEcould be updated to advise on using Dalli, but I'd need to check on the configuration options.